Walkthrough

Adds a shared Shakapacker precompile hook and wiring: dummy and generator hook scripts delegate to a gem-relative shared Ruby hook that detects/builds ReScript and conditionally generates React on Rails packs based on Rails configuration.

Changes

Sequence Diagram(s)

sequenceDiagram
    participant Invoker as Shakapacker
    participant Dummy as Dummy Hook
    participant Shared as Shared Hook
    participant Rails as Rails App
    participant PM as Package Manager
    participant PackGen as Pack Generation

    Invoker->>Dummy: execute precompile hook
    Dummy->>Dummy: resolve gem_root & shared_hook path
    alt shared hook exists
        Dummy->>Shared: load(shared_hook)
        Shared->>Shared: discover Rails root
        opt ReScript present and script defined
            Shared->>PM: run build:rescript (yarn/npm)
            PM-->>Shared: build result
        end
        opt auto-load bundles/components
            Shared->>PackGen: invoke pack generation (rails task / bundle exec)
            PackGen-->>Rails: generate packs
        end
        Shared-->>Invoker: exit success
    else shared hook missing
        Dummy-->>Invoker: warn + exit 1
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Pay extra attention to spec/support/shakapacker_precompile_hook_shared.rb (file/directory detection, package-manager command variants, working-directory changes, and exit codes).
• Verify delegation/guard logic in the dummy hook(s) and template changes to ensure correct gem-relative path resolution across environments.
• Confirm ENV-based validation-skip behavior and CLI messages in the shell template.

Possibly related issues

• Enhancement: Add ReScript support to generator and precompile hook #2043 — This PR implements ReScript detection and build steps in the shared shakapacker precompile hook, matching the issue request.

Possibly related PRs

• Skip generate_packs when shakapacker precompile hook configured #1861 — Related changes to how generate_packs is skipped/handled when a precompile hook is present.
• Fix bin/dev pack generation in Bundler context #1907 — Overlaps in pack-generation invocation paths and bundler/rake execution approaches.
• Upgrade Shakapacker from 8.2.0 to 9.0.0 #1904 — Modifies the Shakapacker precompile hook/configuration similarly (precompile_hook + bin/shakapacker-precompile-hook).

Suggested reviewers

• AbanoubGhadban
• alexeyr-ci
• alexeyr-ci2

Poem

🐰 I hopped through gems to find the hook,
I nudged the script and gave a look,
ReScript builds when files align,
Packs emerge by Rails design,
A shared little hop for precompile delight. 🥕

Pre-merge checks and finishing touches

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Code Review: Fix Pro dummy app Playwright test timeouts

✅ Overall Assessment

This is a well-structured fix that correctly addresses the Playwright test timeout issue by aligning the Pro dummy app with the open-source configuration from PR #1972. The changes are consistent, well-documented, and follow established patterns.

📋 Summary of Changes

1. Changed javascript_pack_tag from async: true to defer: true in application.html.erb
2. Added precompile_hook configuration to shakapacker.yml
3. Added bin/shakapacker-precompile-hook script (executable)

✅ Strengths

1. Root Cause Analysis

• Excellent documentation of the race condition issue
• Clear explanation of why defer: true solves the problem
• Good reference to the original fix in PR Fix component registration race condition with defer loading strategy #1972

2. Code Quality

• Ruby best practices: Uses array form for system() calls for security (lines 76, 80)
• Cross-platform support: Checks for both yarn and npm availability
• Defensive programming: Multiple existence checks before proceeding
• Error handling: Proper exception handling with backtrace output
• RuboCop compliance: Includes rubocop directives for justified complexity

3. Documentation

• Comprehensive inline comments explaining the "why" not just the "what"
• Updated comments in application.html.erb to reflect the new strategy
• Security warning in shakapacker.yml about precompile hook path

4. Consistency

• Matches the open-source dummy app structure
• Uses the same precompile hook script pattern
• Aligns with established project conventions

🔍 Detailed Review

application.html.erb (lines 24-31)

✅ Good: Updated comment accurately describes the defer strategy purpose
✅ Good: Maintains the skip_js_packs test parameter
⚠️ Minor observation: The Pro version has more elaborate layout structure than open-source (includes CSS pack tags and more metadata), but this is expected for Pro features

shakapacker.yml (lines 22-25)

✅ Excellent: Includes security warning about precompile hook
✅ Good: Follows the same configuration pattern as open-source
✅ Good: Uses relative path from project root

bin/shakapacker-precompile-hook (entire file)

✅ Excellent: Robust implementation with multiple safety checks
✅ Security: Uses array form for system calls to prevent shell injection
✅ Cross-platform: Handles both yarn and npm
✅ Fail-fast: Exits with error code 1 on failures
✅ Good UX: Uses emoji indicators for status messages
✅ Smart detection: Regex pattern properly excludes commented config lines (line 65-66)

Specific highlights:

• Line 31-32: Proper use of File::NULL for silent checks
• Line 65-66: Regex correctly matches only active config (not comments)
• Line 76: Uses IO.popen with array form and proper error redirection
• Line 92-101: Clean error handling in main execution block

🐛 Potential Issues

None identified - The code is production-ready.

🔒 Security Review

✅ Command injection prevention: Uses array form for all system() and IO.popen() calls
✅ Path traversal: Uses File.join() for path construction
✅ Input validation: Checks file existence before reading
✅ Secure defaults: Returns early if conditions not met
✅ Documentation: Includes security warning in YAML config

⚡ Performance Considerations

✅ Minimal overhead: Early returns prevent unnecessary work
✅ Efficient checks: Uses simple file existence tests before complex operations
✅ Parallel downloads: defer: true maintains parallel script downloads while ensuring execution order

🧪 Test Coverage

Test Plan Status: ⚠️ Checklist items are unchecked in PR description

Recommendations for testing:

1. ✅ Verify Playwright E2E tests pass in CI (should be automatic)
2. ✅ Confirm ToggleContainer hydrates and logs console message
3. ✅ Check all 11 streaming tests complete successfully
4. Consider adding a test that specifically verifies component registration order
5. Test both development and production builds with the new precompile hook

💡 Suggestions

1. Test Plan Completion
Update the PR description to check off test plan items once CI passes.

2. Verification Script (optional)
Consider adding a simple verification that the hook executes correctly:

# In CI or locally
RAILS_ENV=test bin/shakapacker-precompile-hook
echo "Hook exit code: $?"

3. Documentation (optional, future work)
Consider documenting this pattern in the main React on Rails docs, as the defer vs async strategy is critical for auto_load_bundle users.

📝 Checklist

• ✅ Code follows project conventions (CLAUDE.md)
• ✅ RuboCop compliant (has proper directives)
• ✅ Security best practices followed
• ✅ Cross-platform compatibility
• ✅ Error handling in place
• ✅ Comments explain "why" not just "what"
• ✅ Consistent with related changes (PR Fix component registration race condition with defer loading strategy #1972)
• ⚠️ Test plan needs verification (wait for CI)

🎯 Recommendation

LGTM after CI passes ✅

This is a solid fix that:

• Correctly diagnoses and resolves the root cause
• Follows established patterns from the open-source codebase
• Includes proper security considerations
• Has excellent documentation

Once the Playwright tests pass in CI, this is ready to merge.

📚 References

• Related PR Fix component registration race condition with defer loading strategy #1972: Component registration race condition fix
• Shakapacker precompile hook docs: https://github.com/shakacode/shakapacker/blob/main/docs/precompile_hook.md
• React on Rails auto_load_bundle configuration

Review generated with assistance from Claude Code

Code Review: PR #1977

Overview

This PR successfully addresses Playwright E2E test timeouts in the Pro dummy app by aligning with the defer loading strategy fix from PR #1972. The changes are well-documented and the root cause analysis is thorough.

✅ Strengths

1. Excellent Problem Analysis

• Clear identification of the race condition between component registration and React hydration
• Well-documented root cause linking to the async vs defer loading behavior
• Good reference to the previous fix in PR Fix component registration race condition with defer loading strategy #1972

2. Consistent Implementation

The bin/shakapacker-precompile-hook script is identical between open-source and Pro dummy apps, ensuring consistency across the monorepo. This is excellent for maintainability.

3. Robust Hook Script

The precompile hook demonstrates several best practices:

• Cross-platform compatibility (yarn/npm detection)
• Security-conscious (uses array form for system calls)
• Graceful degradation (silent returns when features aren't needed)
• Proper error handling with exit codes
• Good logging with emoji indicators

4. Security Awareness

The shakapacker.yml includes a security comment about the precompile hook, which is important for preventing malicious hook injection.

🔍 Code Quality Observations

Precompile Hook Script (bin/shakapacker-precompile-hook)

Positives:

• Clean separation of concerns with dedicated functions
• Appropriate RuboCop disables with justification
• Good comments explaining the rationale

Minor Suggestions:

1. Regex Complexity (lines 65-66): The regex /^\s*(?!#).*config\.auto_load_bundle\s*=/ uses negative lookahead which is correct, but could be more precise:

   # Current approach might match commented lines with leading whitespace
   # Consider: /^\s*[^#]*config\.auto_load_bundle\s*=/
   # Or even simpler: check if line.strip starts with '#'

   However, the current implementation should work fine in practice.

2. File.read Performance: Reading the entire initializer file (line 63) is fine for this use case, but be aware this loads the full file into memory. For very large files, consider line-by-line processing (though unlikely to be an issue here).

3. Error Messages: Consider adding the Rails root path to error messages for easier debugging:

   warn "❌ Pack generation failed in #{rails_root}"

View Template Changes (application.html.erb)

Positives:

• Clear comments explaining the rationale for defer: true
• Maintains the skip_js_packs testing parameter

Observation:

• The comment accurately explains that defer ensures proper script execution order for generated component packs

🎯 Test Coverage Considerations

Strong Points:

• The PR targets the exact failing test scenario (line 52 in streaming.spec.ts)
• Test waits for console message 'ToggleContainer with title' which directly validates component hydration

Recommendations:

1. Add Regression Test: Consider adding a specific test case that validates script execution order, perhaps by:

   • Checking that component registrations complete before hydration
   • Verifying no "component not registered" errors in console
   • Testing with multiple generated component packs

2. Test Hook Execution: Add tests for the precompile hook script itself:

   # Test that hook runs successfully
   # Test with/without ReScript config
   # Test with/without React on Rails config

⚡ Performance Considerations

Defer vs Async:

• ✅ Correct choice: defer provides deterministic execution order while maintaining parallel downloads
• ✅ No performance regression: Scripts still download in parallel, only execution order changes
• ✅ Better TTI: Prevents hydration failures which would cause worse performance

Precompile Hook:

• The hook runs on every Shakapacker compilation
• Early returns prevent unnecessary work when features aren't configured
• ✅ Minimal performance impact in normal operation

🔒 Security Considerations

Strengths:

1. ✅ Uses array form for system calls (lines 76, 80) - prevents shell injection
2. ✅ Security comment in shakapacker.yml about trusted scripts
3. ✅ Proper validation before executing commands

Best Practices Followed:

• No use of string interpolation in system calls
• Explicit path checking before file operations
• Fail-fast behavior on errors

📦 Dependency Updates

The Gemfile.lock changes show:

• Shakapacker: 8.0.0 → 9.3.0
• semantic_range: 3.0.0 → 3.1.0
• Open source adds: rbs gem

These dependency updates align with the PR's goals and are consistent with the project's CI configuration (as mentioned in CLAUDE.md).

📝 Documentation & Style

Strengths:

• PR description is comprehensive with clear problem/solution sections
• Code comments are helpful and explain the "why"
• Follows repository conventions from CLAUDE.md

Minor Note:

• Ensure CHANGELOG.md is updated (per CLAUDE.md guidelines) since this is a bug fix that affects users
• Format: [PR 1977](https://github.com/shakacode/react_on_rails/pull/1977) by [justin808](https://github.com/justin808)

✨ Additional Recommendations

1. DRY Opportunity: The precompile hook script is duplicated between open-source and Pro. Consider:

   • Symlinking to a shared script
   • Or documenting why duplication is intentional (if Pro needs to diverge in the future)

2. Monitoring: Consider adding telemetry or logging to track:

   • How often the race condition would have occurred (if async were still used)
   • Hook execution time for performance monitoring

3. Documentation Update: Update any documentation that mentions async: true as the recommended approach for Shakapacker >= 8.2.0

🎬 Conclusion

Overall Assessment: ✅ LGTM with minor suggestions

This is a solid fix that:

• ✅ Addresses the root cause correctly
• ✅ Follows project conventions
• ✅ Maintains consistency across the monorepo
• ✅ Includes proper error handling and security considerations
• ✅ Has comprehensive documentation

The code quality is high and the approach is sound. The minor suggestions above are optional improvements that don't block merging.

Recommendation: Approve and merge after:

1. CI passes (especially the Playwright E2E tests this targets)
2. CHANGELOG.md is updated
3. Consider the DRY opportunity for the precompile hook script

Great work on the thorough analysis and consistent implementation! 🚀

Code Review - PR #1977

Summary

This PR successfully addresses Playwright E2E test timeouts in the Pro dummy app by aligning it with the defer loading strategy from PR #1972. The changes are well-motivated, properly documented, and follow established patterns from the open-source codebase.

Positive Aspects

1. Excellent Problem Analysis ✅

The PR description clearly explains:

• The specific symptom (30-second timeout waiting for console message)
• The root cause (race condition between async script loading)
• Why the fix works (defer ensures proper execution order)

2. Code Consistency ✅

• Changes mirror the successful fix from PR Fix component registration race condition with defer loading strategy #1972
• The shakapacker-precompile-hook script is identical to the OS version (verified by reading both)
• Maintains consistency between open-source and Pro packages

3. Security Awareness ✅

The shakapacker.yml includes appropriate security documentation:

# SECURITY: Only reference trusted scripts within your project. Ensure the hook path
# points to a file within the project root that you control.

4. Robust Hook Implementation ✅

The precompile hook demonstrates good practices:

• Cross-platform compatibility (works with both yarn/npm)
• Graceful degradation (returns early if dependencies not found)
• Proper error handling with exit codes
• Security-conscious (uses array form for system commands)
• RuboCop compliant (disables only necessary complexity check with explanation)

Observations & Minor Suggestions

1. Comment Update in Layout ⚠️

The old comment in application.html.erb (lines 27-31 in current file) still references async: true and discusses Selective Hydration. While the new comment (lines 27-29 in the diff) is more accurate for the defer strategy, consider:

Before (lines 27-31):

<%# async: true is the recommended approach for Shakapacker >= 8.2.0 (currently using 9.3.0).
    It enables React 18's Selective Hydration and provides optimal Time to Interactive (TTI).
    Use immediate_hydration feature to control hydration timing for Selective/Immediate Hydration.
    See docs/building-features/streaming-server-rendering.md
    skip_js_packs param is used for testing purposes to simulate hydration failure %>

After (lines 27-29 in diff):

<%# Use defer: true to ensure proper script execution order.
    When using generated component packs (auto_load_bundle), defer ensures
    component registrations complete before React hydration begins.

Suggestion: The skip_js_packs usage context is lost in the new comment. Consider adding:

<%# Use defer: true to ensure proper script execution order.
    When using generated component packs (auto_load_bundle), defer ensures
    component registrations complete before React hydration begins.
    skip_js_packs param is used for testing purposes to simulate hydration failure %>

2. Gemfile.lock Changes ℹ️

The PR includes Shakapacker version bump from 8.0.0 → 9.3.0 and semantic_range updates. These appear intentional for aligning with the "Latest" CI configuration, but worth confirming:

• Is this a deliberate upgrade as part of CI alignment?
• Have all Shakapacker 9.3.0 compatibility concerns been addressed?

3. Test Verification 📋

The test plan checklist is still unchecked. Ensure:

• Playwright E2E tests pass in CI
• ToggleContainer console message appears (line 52 in streaming.spec.ts)
• All 11 streaming tests complete without timeouts

4. rbs Gem Addition ℹ️

The open-source Gemfile.lock shows rbs was added as a dependency. This seems unrelated to the defer loading fix - was this intentional or a side effect of bundle operations?

Code Quality Assessment

Security ✅

• Hook script uses secure command execution patterns (system with array form)
• Proper path validation for Rails root detection
• Security warnings documented in config

Performance ✅

• Defer loading maintains parallel download benefits while fixing execution order
• Hook runs only when needed (checks for config presence)
• Early returns prevent unnecessary work

Test Coverage ✅

• Existing Playwright tests (streaming.spec.ts:52) will verify the fix
• Tests specifically check for the console message that was timing out

Best Practices ✅

• Follows repository conventions per CLAUDE.md
• Ruby script has proper shebang and frozen_string_literal
• Cross-platform compatibility considered
• Error handling with meaningful messages

Potential Issues

None Identified 🎉

No bugs, security vulnerabilities, or significant issues found. The implementation is solid.

Recommendations

1. Verify CI Passes: Ensure all Playwright tests complete successfully
2. Update Comment: Consider preserving the skip_js_packs documentation in the ERB comment
3. Confirm Shakapacker Upgrade: Verify the 8.0.0 → 9.3.0 upgrade is intentional and complete
4. Check Test Plan: Mark off the checklist items in the PR description once verified

Conclusion

Recommendation: Approve with minor suggestions ✅

This is a well-executed fix that:

• Solves a real production issue (test timeouts)
• Follows established patterns from PR Fix component registration race condition with defer loading strategy #1972
• Maintains code quality and consistency
• Includes proper documentation and error handling

The minor suggestions above are non-blocking improvements. Great work tracking down the race condition and applying the consistent fix across both codebases!

Review conducted using repository guidelines from CLAUDE.md

Code Review: PR #1977

Overview

This PR fixes Playwright E2E test timeouts in the Pro dummy app by applying the same defer loading strategy from #1972. The changes are well-motivated and align the Pro dummy app with the open-source configuration.

✅ Strengths

1. Root Cause Analysis: Excellent problem diagnosis. The PR clearly identifies the race condition where async: true caused unpredictable script execution order, leading to React hydration failures.

2. Consistent Solution: Mirrors the fix from Fix component registration race condition with defer loading strategy #1972 (commit d1a8a1a), maintaining consistency across the monorepo.

3. Comprehensive Fix: Addresses both the immediate issue (defer loading) and the underlying build automation (precompile hook).

4. Good Documentation: Clear comments in the ERB template explaining why defer: true is needed for auto-generated component packs.

5. Executable Permissions: The precompile hook has correct permissions (755).

6. Cross-platform Compatibility: The precompile hook script handles multiple scenarios (yarn/npm, bundle availability) gracefully.

🔍 Issues & Recommendations

1. Security: Command Injection Risk (Medium Priority)

Location: bin/shakapacker-precompile-hook:35-37

success = if yarn_available
            system("yarn", "build:rescript")
          elsif npm_available
            system("npm", "run", "build:rescript")

Issue: The script uses the array form of system() which is secure, but this should be explicitly documented as a security best practice since this hook runs during build/deployment.

Recommendation: Add a comment explaining the security consideration:

# Use array form to prevent shell injection
success = if yarn_available
            system("yarn", "build:rescript")

2. Error Handling: Silent Failures (Low Priority)

Location: bin/shakapacker-precompile-hook:39-40

else
  warn "⚠️  Warning: Neither yarn nor npm found. Skipping ReScript build."
  return

Issue: When neither package manager is found, the script warns but continues. If ReScript is required, this could lead to broken builds that fail later.

Recommendation: Consider making this configurable or failing fast if ReScript config exists:

else
  warn "❌ Error: ReScript config found but no package manager available"
  exit 1
end

3. Code Quality: Complex Regex (Low Priority)

Location: bin/shakapacker-precompile-hook:65-66

has_auto_load = config_file =~ /^\s*(?\!#).*config\.auto_load_bundle\s*=/
has_components_subdir = config_file =~ /^\s*(?\!#).*config\.components_subdirectory\s*=/

Issue: The negative lookahead (?\!#) doesn't correctly skip comments. A line like # config.auto_load_bundle = true would match because the lookahead only checks the first character after whitespace.

Recommendation: Use a more robust regex:

# Match lines that don't start with # (after optional whitespace)
has_auto_load = config_file =~ /^\s*[^#]*config\.auto_load_bundle\s*=/
has_components_subdir = config_file =~ /^\s*[^#]*config\.components_subdirectory\s*=/

Or use Ruby parsing:

has_auto_load = config_file.lines.any? { |line| line.strip[0] \!= '#' && line.include?('config.auto_load_bundle =') }

4. Missing Test Coverage (Medium Priority)

Issue: No tests for the precompile hook script. Given its critical role in the build process, it should have unit tests.

Recommendation: Add tests in react_on_rails_pro/spec/dummy/spec/bin/shakapacker_precompile_hook_spec.rb:

• Test ReScript build detection
• Test Rails root finding
• Test config parsing (especially edge cases with comments)
• Test error handling

5. Documentation: Shakapacker Version (Low Priority)

Location: config/shakapacker.yml:25

The comment references Shakapacker docs, but doesn't specify the minimum version required for precompile_hook support.

Recommendation: Add version requirement to the comment:

# Hook to run before webpack compilation (requires Shakapacker >= X.X.X)

🧪 Testing Considerations

The PR description mentions a test plan but doesn't show completed checkboxes. Before merging:

• ✅ Verify CI passes (especially the Playwright streaming tests)
• ✅ Confirm ToggleContainer console message appears (check CI logs)
• ✅ Test the precompile hook runs correctly in both development and production builds
• ✅ Verify the hook doesn't break builds when ReScript isn't present

🎯 Performance Considerations

Positive: Using defer: true maintains parallel download performance while ensuring deterministic execution order. This is the optimal strategy for this use case.

Neutral: The precompile hook adds build time overhead, but this is necessary for auto-generated packs.

📋 Summary

Approve with minor suggestions. The core fix is sound and well-implemented. The issues identified are mostly improvements rather than blockers:

• Must fix: None (code is mergeable)
• Should fix: Add test coverage for the precompile hook
• Nice to have: Improve regex robustness, add security comments

The PR successfully addresses the test timeout issue and aligns the Pro dummy app with the open-source configuration. Great work documenting the root cause and solution!

📚 References

• Related PR: Fix component registration race condition with defer loading strategy #1972
• Shakapacker precompile hook docs: https://github.com/shakacode/shakapacker/blob/main/docs/precompile_hook.md
• File locations:
  • react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb:32
  • react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
  • react_on_rails_pro/spec/dummy/config/shakapacker.yml:25

Code Review for PR #1977

Thank you for this PR! The changes effectively address the Playwright test timeout issue by aligning the Pro dummy app with the open-source configuration. Here's my detailed review:

✅ Strengths

1. Root Cause Analysis is Solid

The PR correctly identifies the race condition caused by async: true loading strategy where component registration might not complete before React hydration begins. The switch to defer: true is the right solution.

2. Consistency Across Codebase

Aligning the Pro dummy app with the open-source dummy app (from #1972) ensures consistent behavior and reduces maintenance burden.

3. Comprehensive Solution

The PR includes all necessary pieces:

• Changed loading strategy in application.html.erb
• Added precompile hook for automatic pack generation
• Updated configuration files
• Added helpful comments explaining the rationale

4. Good Documentation

The inline comments explain why defer: true is used, which is crucial for future maintainers.

🔍 Observations & Suggestions

1. Code Duplication: Precompile Hook

The shakapacker-precompile-hook file in the Pro dummy app is nearly identical to the one in the open-source dummy app, with one difference:

Pro version (line 75):

task_list = IO.popen(["bundle", "exec", "rails", "-T"], err: %i[child out], &:read)

Open-source version (line 76):

task_list = IO.popen(["bundle", "exec", "rails", "-T"], err: [:child, :out], &:read)

The Pro version uses %i[child out] (symbol array literal) while the open-source version uses [:child, :out] (explicit array). Both are functionally equivalent.

Suggestion: Consider one of these approaches to reduce duplication:

• Extract the hook to a shared location (e.g., lib/generators/react_on_rails/templates/)
• Add a note in both files indicating they should be kept in sync
• Use a symlink if the codebase structure supports it

2. RuboCop Violation in Pro Version

The open-source version has a RuboCop disable comment for Metrics/CyclomaticComplexity (lines 52, 89), but the Pro version doesn't. This might cause CI failures if RuboCop runs on the Pro package.

Action Required:

# Add these lines to react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
# rubocop:disable Metrics/CyclomaticComplexity
def generate_packs_if_needed
  # ... existing code ...
end
# rubocop:enable Metrics/CyclomaticComplexity

3. Security Note is Well-Placed

The security comment in shakapacker.yml about trusting the precompile hook script is excellent. This is important for preventing arbitrary code execution.

4. Test Coverage

The PR description mentions test timeouts were the issue, and the fix ensures proper hydration. However:

• Question: Have you verified that all 11 streaming tests now pass consistently?
• Suggestion: Consider adding a test that explicitly verifies the script loading order (though this might be overkill for this issue)

5. Comment Accuracy in react_on_rails.rb

Lines 42-43 in react_on_rails_pro/spec/dummy/config/initializers/react_on_rails.rb:

# Don't explicitly set generated_component_packs_loading_strategy - let it default to :defer
# which ensures generated component packs load and register components before main bundle executes

This is good documentation. However, it might be worth explicitly stating that this default behavior changed or was introduced in a specific version for future reference.

🐛 Potential Issues

1. Missing RuboCop Pragmas (Critical)

As mentioned above, the Pro version is missing the RuboCop disable comment that exists in the open-source version. This will cause CI failures based on the CLAUDE.md requirements:

BEFORE EVERY COMMIT/PUSH:

1. ALWAYS run bundle exec rubocop and fix ALL violations

Priority: High - Should be fixed before merging.

2. File Permissions

The script needs to be executable. Based on the diff, the file is new. Ensure it has execute permissions:

chmod +x react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook

Git should preserve this, but worth verifying.

🎯 Performance Considerations

Script Loading Performance

Before (async): Scripts load in parallel, potentially faster initial load but risk of race conditions.
After (defer): Scripts load in parallel but execute in order, slightly slower but reliable.

Trade-off is acceptable because:

• The performance difference is negligible (milliseconds)
• Correctness > speed in this case
• User experience improves due to proper hydration

Precompile Hook Performance

The hook runs during precompilation, not runtime, so performance impact is minimal. The conditional checks (file existence, config parsing) are efficient.

🔒 Security Considerations

Precompile Hook Execution

✅ Good: The hook uses array form for system calls (system("bundle", "exec", "rails", ...)), which prevents shell injection.
✅ Good: Security warning in shakapacker.yml alerts users to only use trusted scripts.
✅ Good: Hook validates command availability before execution.

No security concerns identified.

📋 Required Actions Before Merge

1. Add RuboCop pragmas to react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook (lines 52 and 87)
2. Run bundle exec rubocop on the Pro package to verify no violations
3. Verify all Playwright tests pass in CI
4. Confirm file permissions are correct for the hook script

📝 Optional Improvements

1. Consider documenting the code duplication between Pro and open-source hook scripts
2. Add a test that verifies the precompile hook runs successfully during asset compilation
3. Consider adding a comment in the open-source hook pointing to the Pro version and vice versa

✨ Overall Assessment

Rating: Approve with minor changes

This is a well-researched fix that addresses a real issue with a proven solution. The root cause analysis is thorough, and the implementation is sound. The only blocking issue is the missing RuboCop pragma, which should be a quick fix.

Recommended next steps:

1. Add the RuboCop disable/enable comments
2. Run bundle exec rubocop to verify
3. Confirm CI passes
4. Merge

Great work on identifying and fixing this race condition! 🚀

Code Review

Summary

This PR converts the precompile hook from Ruby to POSIX shell script, which is a solid improvement for cross-platform compatibility and reduces dependencies. The implementation is clean and well-structured. However, there are several important issues that need to be addressed.

Critical Issues

1. Shell Script Execution Issue with set -e

Location: Lines 13, 44, 97, 53, 61

Problem: The script uses set -e which causes the script to exit immediately on any error. However, within functions, there are conditional blocks that check command success:

if yarn build:rescript; then
  echo "✅ ReScript build completed successfully"
  return 0
else
  echo "❌ ReScript build failed" >&2
  exit 1
fi

With set -e, if yarn build:rescript fails, the script will exit before reaching the else block. This makes the error handling logic unreachable.

Impact: The custom error messages ("❌ ReScript build failed") will never be displayed. The script will just exit silently with a non-zero status.

Fix: Either:

• Remove set -e and rely on explicit error checking in if/else blocks
• Use set -e with || { echo "error"; exit 1; } patterns
• Use trap for error handling

2. Directory Change Side Effect

Location: Lines 44, 97

Problem: Both functions use cd "$rails_root" which permanently changes the working directory for the rest of the script execution. If build_rescript_if_needed() changes to Rails root, then generate_packs_if_needed() will be running from a different directory than expected.

Impact: Could cause issues if the script is called from a subdirectory and subsequent operations depend on the original working directory.

Fix: Either:

• Use subshells: (cd "$rails_root" && yarn build:rescript)
• Save and restore the original directory
• Document the behavior clearly

Medium Priority Issues

3. Missing ReScript Build Script Validation

Location: Lines 47-62

Problem: The script assumes build:rescript exists in package.json but doesn't verify this before attempting to run it.

Impact: If a project has rescript.json but no build:rescript script, you'll get a confusing error message from yarn/npm instead of a helpful message from the hook.

Suggestion: Add validation:

if \! grep -q '"build:rescript"' "$rails_root/package.json"; then
  echo "⚠️  Warning: ReScript config found but no build:rescript script in package.json"
  return 0
fi

4. Grep Pattern May Match Comments

Location: Lines 84-85

Problem: While the regex ^[[:space:]]*config\. attempts to exclude comments, it could match code within multi-line comments or commented-out code that looks like:

# config.auto_load_bundle = true  # This is commented but will match\!

Suggestion: Consider a more robust check or add documentation about this limitation.

5. bundle exec rails -T Performance

Location: Line 100

Problem: Running rails -T loads the entire Rails environment just to check if a rake task exists. This adds significant overhead (1-3 seconds) to every webpack compilation.

Impact: Slows down development workflow, especially with hot reload.

Suggestion:

• Cache the result in a temp file with timestamp
• Or just try running the task and handle the error gracefully
• Or check for the existence of the rake task file directly

Minor Issues / Suggestions

6. Inconsistent Emoji Usage

The script uses emojis in output, which may not render correctly in all terminals or CI environments. Consider documenting this or making it optional.

7. Error Messages to stderr

Good practice using >&2 for error messages on lines 52, 60, 113. However, warnings (lines 32, 64) should also go to stderr for consistency.

8. Redundant Return Statements

Lines 50, 58, 111 have return 0 before exit 1 in else blocks - the return is unreachable. Not a bug, just unnecessary.

Security Considerations

✅ Good: The script uses proper quoting around variables ("$rails_root")
✅ Good: The shakapacker.yml includes security comments about trusting the hook path
✅ Good: No user input is directly executed
✅ Good: Commands use direct invocation (not eval or string-based execution)

Testing Recommendations

The PR checklist shows several test items are unchecked:

• Verify hook runs during bin/shakapacker compilation
• Confirm ReScript builds when config files present
• Check pack generation when auto_load_bundle configured
• Test production asset precompilation workflow

These tests are critical - please complete them before merging. The set -e issue (#1) will only be caught by actual execution testing.

Suggested test scenarios:

1. Test when ReScript config exists but build:rescript script is missing
2. Test when called from a subdirectory
3. Test when Rails root cannot be found
4. Test when bundle is not available
5. Test error propagation to webpack/shakapacker

Positive Aspects

✅ Excellent migration from Ruby to POSIX shell - much lighter weight
✅ Good graceful degradation (warnings instead of errors when optional tools missing)
✅ Clear, readable code with good comments
✅ Cross-platform package manager detection
✅ Security-conscious implementation
✅ Proper use of REACT_ON_RAILS_SKIP_VALIDATION environment variable

Recommendation

Request Changes - The set -e issue (#1) is critical and will cause silent failures. The directory change issue (#2) could cause subtle bugs. Please address these before merging.

Once fixed, this is a solid improvement to the codebase. The shell script approach is definitely the right direction for a precompile hook.

cc @justin808

Code Review - PR #1977

Summary

This PR refactors the Shakapacker precompile hook to add ReScript build support and consolidate duplicate code across dummy apps. The changes follow good DRY principles and improve the developer experience.

✅ Strengths

1. Excellent Code Reuse

The creation of shakapacker_precompile_hook_shared.rb eliminates duplication between the two test dummy apps. The wrapper scripts are clean and minimal.

2. Robust Error Handling

• Graceful degradation with warnings when dependencies are missing
• Proper exception handling with informative error messages
• Exits with proper error codes (exit 1) on failures

3. Security Improvements

• Uses array form for system() calls to prevent shell injection
• Validates script existence before loading
• Good security comment in shakapacker.yml about trusted script paths

4. Cross-Platform Compatibility

• Package manager detection works for both yarn and npm
• Portable shell command detection using which

5. Good Documentation

• Clear comments explaining the purpose and usage
• Links to Shakapacker documentation
• Helpful emoji indicators for user feedback

🐛 Issues Found

1. Critical: Regex Pattern Missing Comment Exclusion

Location: lib/tasks/precompile/shakapacker_precompile_hook_shared.rb:88-89

The regex patterns for detecting config need to exclude comments:

# Current (INCORRECT - matches commented lines):
return unless initializer_content.match?(/^\s*config\.auto_load_bundle\s*=/) ||
              initializer_content.match?(/^\s*config\.components_subdirectory\s*=/)

# Should be (matches only uncommented lines):
return unless initializer_content.match?(/^\s*(?\!#).*config\.auto_load_bundle\s*=/) ||
              initializer_content.match?(/^\s*(?\!#).*config\.components_subdirectory\s*=/)

This is inconsistent with the generator template version (lines 37-38), which correctly excludes comments. This could cause the hook to run even when auto-pack generation is commented out.

2. Platform Compatibility Issue: which Command

Location: lib/tasks/precompile/shakapacker_precompile_hook_shared.rb:61,63

Using which with shell redirection in system() is problematic:

# Current (shell-dependent):
if system("which yarn > /dev/null 2>&1")

# Better (cross-platform):
if system("yarn", "--version", out: File::NULL, err: File::NULL)

Why this matters:

• The current approach passes the entire string to the shell, which is less portable
• Windows doesn't have which by default
• The improved version uses Ruby's built-in null device and array form

3. Potential Race Condition with load vs require

Location: spec/dummy/bin/shakapacker-precompile-hook:19 and Pro version

Using load instead of require means:

• The shared script is executed every time, even if already loaded
• Functions are redefined on each load
• For this use case, this is probably intentional and fine
• However, consider documenting why load is used instead of require

This is likely correct behavior for a hook script (you want fresh execution), but worth documenting.

4. Missing JSON Parse Error Handling

Location: lib/tasks/precompile/shakapacker_precompile_hook_shared.rb:52

package_json = JSON.parse(File.read(package_json_path))

Should handle malformed JSON:

begin
  package_json = JSON.parse(File.read(package_json_path))
rescue JSON::ParserError => e
  warn "⚠️  Warning: Invalid package.json format: #{e.message}"
  return
end

5. Inconsistent Error Handling Between Functions

Location: lib/tasks/precompile/shakapacker_precompile_hook_shared.rb

• build_rescript_if_needed catches StandardError and exits with error code (lines 72-74)
• generate_packs_if_needed catches both Errno::ENOENT (warns only) and StandardError (exits)

This asymmetry could be confusing. Consider:

• Why does Errno::ENOENT only warn in pack generation but not in ReScript build?
• Should both functions have consistent error handling?

🔍 Performance Considerations

1. File I/O Optimization

Location: lib/tasks/precompile/shakapacker_precompile_hook_shared.rb

Multiple file reads could be optimized:

• find_rails_root walks the directory tree twice (once for ReScript, once for packs)
• Consider caching the Rails root in a module variable if performance becomes an issue

Current impact: Negligible for typical use, but worth noting.

🧪 Test Coverage Concerns

Missing Test Coverage

The PR description shows test plan checkboxes, but several are unchecked:

• Verify hook runs during bin/shakapacker compilation
• Confirm ReScript builds when config files present
• Check pack generation when auto_load_bundle configured
• Test production asset precompilation workflow

Recommendation: Add integration tests or document manual testing results before merging.

Suggested Test Scenarios

1. ReScript detection:

   • With bsconfig.json only
   • With rescript.json only
   • With both files
   • With config but no build:rescript script
   • With invalid package.json

2. Pack generation:

   • With commented config (should NOT run)
   • With uncommented config (should run)
   • Without Rails initializer

3. Error cases:

   • Missing Rails root
   • Missing package manager
   • Failed ReScript build
   • Failed pack generation

🛡️ Security Considerations

✅ Good Security Practices

1. Uses array form for system() calls - prevents shell injection
2. Validates file existence before loading
3. Clear security comment in config file
4. Proper path resolution without user input

⚠️ Minor Concern: Command Execution

The hook executes arbitrary rake tasks and npm scripts. This is acceptable because:

• Scripts are defined in the project's own files
• No user input is interpolated
• The security comment in shakapacker.yml warns about this

📝 Code Style & Conventions

✅ Follows React on Rails Conventions

• Proper RuboCop disables with justification
• Frozen string literal comments
• Good emoji usage for user feedback
• Proper newlines at end of files

Minor Style Notes

1. Inconsistent loop style:

   • Generator template uses while loop (lines 18-23)
   • Shared implementation uses while loop (lines 21-27)
   • Both are fine, good consistency

2. Comment quality: Excellent throughout

🎯 Recommendations

High Priority (Fix Before Merge)

1. ✅ Fix regex to exclude commented config lines (consistency bug)
2. ✅ Replace which with cross-platform package manager detection
3. ✅ Add JSON parse error handling

Medium Priority

4. Document why load is used instead of require
5. Make error handling consistent between functions
6. Complete manual testing checklist

Low Priority

7. Add integration tests for the hook
8. Consider caching Rails root if performance becomes an issue

📋 Checklist Verification

According to CLAUDE.md requirements:

• ✅ RuboCop passes (mentioned in PR description)
• ✅ Pre-commit hooks pass (mentioned in PR description)
• ✅ Files end with newlines (verified in diff)
• ❓ Need to verify: All tests passing
• ❓ Need to verify: Changelog entry (if user-visible feature)

🎓 Overall Assessment

Quality: Good ⭐⭐⭐⭐ (4/5)

Verdict: Approve with minor fixes required

This is a well-structured refactoring that improves code reuse and adds valuable ReScript support. The main issues are:

1. A regex inconsistency that could cause incorrect behavior
2. Minor cross-platform compatibility improvements needed
3. Test coverage needs verification

The code demonstrates good practices overall - security-conscious, well-documented, and following the project's conventions.

💬 Questions for Author

1. Was the regex difference between the generator template and shared implementation intentional?
2. Have you tested this with an actual ReScript project?
3. Should we add integration tests for the precompile hook?
4. Is there a specific reason for catching Errno::ENOENT separately in generate_packs_if_needed?

Generated by Claude Code Review

/run-skipped-ci

🚀 Full CI Mode Enabled

✅ All checks are already running - nothing to do!

All CI checks are already running on this PR. Added the full-ci label - future commits will run the full CI suite.

Code Review: Shakapacker Precompile Hook Implementation

Thank you for this well-structured PR! The implementation shows good evolution through the commit history. Here's my detailed review:

✅ Strengths

1. Excellent Code Organization

   • Smart separation between test dummy apps (shared Ruby implementation) and user-facing template (simple bash script)
   • DRY principle applied effectively - eliminated 342 lines of duplicate code
   • Clear comments explaining the purpose and usage

2. Robust Error Handling (Shared Implementation)

   • Comprehensive validation for ReScript builds (config exists → package.json exists → build script exists)
   • Actionable error messages that tell users exactly how to fix issues
   • Proper exit codes on failures

3. Cross-Platform Support

   • Works with both yarn and npm
   • Handles both legacy (bsconfig.json) and modern (rescript.json) ReScript configs

4. Security Conscious

   • Uses system(..., exception: true) to properly propagate errors
   • Includes security comment in shakapacker.yml about trusted script paths
   • Template uses set -e for fail-fast behavior

5. Great Evolution

   • Commit history shows thoughtful iterations based on feedback
   • Final design balances simplicity (template) with robustness (test apps)

🔍 Issues & Suggestions

1. Regex Pattern Robustness (Critical)

Location: spec/support/shakapacker_precompile_hook_shared.rb:95-96

The current regex pattern has a subtle bug:

# Current (problematic)
return unless initializer_content.match?(/^\s*(?\!#).*config\.auto_load_bundle\s*=/) ||
              initializer_content.match?(/^\s*(?\!#).*config\.components_subdirectory\s*=/)

Problem: The negative lookahead (?\!#) only checks that the character immediately after whitespace isn't #, but it doesn't prevent matching lines where # appears later. This will match:

  # config.auto_load_bundle = true  # ← This WILL match (false positive)
  x = 1 # config.auto_load_bundle = true  # ← This WILL match (false positive)

Solution: Use a proper pattern that ensures the line doesn't start with #:

# Fixed version
return unless initializer_content.match?(/^\s*[^#\s].*config\.auto_load_bundle\s*=/) ||
              initializer_content.match?(/^\s*[^#\s].*config\.components_subdirectory\s*=/)

This pattern:

• ^\s* - Start of line + optional whitespace
• [^#\s] - First non-whitespace character must NOT be # or whitespace
• .*config\. - Followed by anything, then the config

Same issue exists in the bash template (lib/generators/react_on_rails/templates/base/base/bin/shakapacker-precompile-hook:14-15):

# Current (has same issue)
if grep -E "^[[:space:]]*[^#]*config\.auto_load_bundle[[:space:]]*=" ...

# Fixed version
if grep -E "^[[:space:]]*[^#[:space:]].*config\.auto_load_bundle[[:space:]]*=" ...

2. Rails Root Discovery Logic (Medium Priority)

Location: spec/support/shakapacker_precompile_hook_shared.rb:20-28

def find_rails_root
  dir = Dir.pwd
  while dir \!= "/"
    return dir if File.exist?(File.join(dir, "config", "environment.rb"))
    dir = File.dirname(dir)
  end
  nil
end

Issues:

1. Windows Compatibility: The check dir \!= "/" won't work on Windows where root paths are like C:\\
2. Infinite Loop Risk: On Windows, this could loop forever

Solution:

def find_rails_root
  dir = Dir.pwd
  loop do
    return dir if File.exist?(File.join(dir, "config", "environment.rb"))
    
    parent = File.dirname(dir)
    return nil if parent == dir  # Reached filesystem root (works cross-platform)
    
    dir = parent
  end
end

This pattern:

• Works on both Unix (/) and Windows (C:\\)
• Exits when File.dirname(dir) == dir (universal root detection)
• Clearer intent with explicit loop control

3. Package Manager Detection (Low Priority)

Location: spec/support/shakapacker_precompile_hook_shared.rb:63

if system("which yarn > /dev/null 2>&1")

Issues:

1. which isn't available on all systems (especially Windows)
2. Mixing Ruby and shell commands when Ruby has better alternatives

Better Approach:

# More robust package manager detection
yarn_available = system("yarn", "--version", out: File::NULL, err: File::NULL)
npm_available = system("npm", "--version", out: File::NULL, err: File::NULL)

if yarn_available
  system("yarn", "build:rescript", exception: true)
elsif npm_available
  system("npm", "run", "build:rescript", exception: true)
else
  warn "❌ Error: Neither yarn nor npm found but ReScript build required"
  warn "    Install yarn or npm to build ReScript files"
  exit 1
end

This is:

• Cross-platform (no which dependency)
• Pure Ruby (no shell parsing)
• Already used in your code for Rails root resolution!

4. Error Message Consistency (Low Priority)

The error handling is inconsistent between ReScript builds and pack generation:

ReScript build: Exits with status 1 on failure (correct)
Pack generation:

• Line 109: warn "⚠️ Warning: #{e.message}" (but doesn't exit)
• Line 112: warn "❌ ... failed" then exit 1

Issue: The Errno::ENOENT rescue block (line 108-109) issues a warning but continues, which seems incorrect. If pack generation is required but fails due to missing file, shouldn't it exit?

Suggestion: Either:

1. Make both operations fail-fast consistently, or
2. Add comments explaining why ENOENT is non-fatal for pack generation

5. Missing Test Coverage Consideration

The PR test plan shows:

• RuboCop passes
• Pre-commit hooks pass
• Runtime testing items unchecked

Recommendation: Before merging, verify:

1. Hook executes during bin/shakapacker compilation
2. ReScript builds when config present
3. Pack generation triggers when configured
4. Production precompilation workflow works

Consider adding a simple test that validates the shared hook logic, especially the regex matching.

🔒 Security Assessment

✅ No security concerns identified

• Proper use of array form for system calls (prevents shell injection)
• No user input directly interpolated into commands
• Environment variable setting (REACT_ON_RAILS_SKIP_VALIDATION) is appropriate
• Security comment in shakapacker.yml appropriately warns about hook path trust

🚀 Performance Considerations

✅ Efficient implementation

• Early returns prevent unnecessary work
• File existence checks before expensive operations
• Rails root cached in local variable
• No N+1 style issues

📝 Documentation & Code Style

Strengths:

• Clear comments explaining purpose
• Emoji scheme makes output user-friendly
• Good function naming

Minor Suggestion:
Consider adding a one-line comment in the template explaining why it's bash vs Ruby:

#\!/bin/sh
# Simple bash implementation for production apps (no Ruby startup overhead)
# Shakapacker precompile hook for React on Rails

🎯 Summary

This is a well-architected PR that thoughtfully solves the precompile hook problem. The main issues are:

1. Critical: Regex pattern for detecting config (false positives possible)
2. Medium: Rails root discovery won't work on Windows
3. Low: Package manager detection could be more robust
4. Low: Error handling consistency

Recommendation: Fix the regex pattern (critical issue) before merging. The Windows compatibility issue is lower priority unless Windows support is a goal.

The code quality is high, and the evolution through commits shows great attention to feedback and iterative improvement. Excellent work! 🎉

Files Reviewed:

• ✅ spec/support/shakapacker_precompile_hook_shared.rb
• ✅ spec/dummy/bin/shakapacker-precompile-hook
• ✅ react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
• ✅ lib/generators/react_on_rails/templates/base/base/bin/shakapacker-precompile-hook
• ✅ react_on_rails_pro/spec/dummy/config/shakapacker.yml

Code Review for PR #1977

Thank you for this well-structured PR! The precompile hook implementation is solid and follows good practices. Here's my detailed review:

✅ Strengths

1. Excellent Code Organization

   • DRY approach with shared implementation in spec/support/shakapacker_precompile_hook_shared.rb
   • Clear separation between test dummy apps and production template
   • Well-documented with helpful comments

2. Smart Architecture Decisions

   • Generator template uses simple, standalone Ruby script that loads Rails environment
   • Leverages PacksGenerator.generate_packs_if_stale which already handles staleness detection
   • Test dummy apps share common implementation to reduce duplication

3. Security Considerations

   • Uses array form for system calls (prevents shell injection)
   • Proper security comment in shakapacker.yml about trusted script paths
   • No arbitrary code execution vulnerabilities

4. Error Handling

   • Comprehensive error messages with actionable guidance
   • Proper exit codes on failures
   • Graceful fallbacks where appropriate

5. Cross-Platform Support

   • Package manager detection (yarn vs npm)
   • Portable path resolution using File.join
   • Works across development and production environments

🔍 Potential Issues & Suggestions

1. Race Condition in find_rails_root (Minor)

Location: spec/support/shakapacker_precompile_hook_shared.rb:20-28

The current implementation stops at / but could fail on Windows (uses C:\\, D:\\, etc.).

# Current
def find_rails_root
  dir = Dir.pwd
  while dir \!= "/"  # ⚠️ Won't work on Windows
    return dir if File.exist?(File.join(dir, "config", "environment.rb"))
    dir = File.dirname(dir)
  end
  nil
end

Suggestion:

def find_rails_root
  dir = Dir.pwd
  loop do
    return dir if File.exist?(File.join(dir, "config", "environment.rb"))
    
    parent = File.dirname(dir)
    return nil if parent == dir  # Reached filesystem root (works on all OSes)
    
    dir = parent
  end
end

This matches the pattern you had in an earlier commit and is more robust.

2. Package Manager Detection Uses Shell Command (Minor)

Location: spec/support/shakapacker_precompile_hook_shared.rb:63-66

The current implementation uses shell commands for detection:

if system("which yarn > /dev/null 2>&1")
  system("yarn", "build:rescript", exception: true)
elsif system("which npm > /dev/null 2>&1")

Issues:

• which doesn't exist on Windows (should use where instead)
• Shell redirection in system call is less portable

Suggestion:

# More portable approach
yarn_available = system("yarn", "--version", out: File::NULL, err: File::NULL)
npm_available = system("npm", "--version", out: File::NULL, err: File::NULL)

if yarn_available
  system("yarn", "build:rescript", exception: true)
elsif npm_available
  system("npm", "run", "build:rescript", exception: true)
else
  warn "❌ Error: Neither yarn nor npm found but ReScript build required"
  exit 1
end

This approach:

• Works on Windows, Linux, and macOS
• Uses Ruby's built-in File::NULL for cross-platform output suppression
• More consistent with your earlier code patterns

3. Regex Could Be More Robust (Low Priority)

Location: spec/support/shakapacker_precompile_hook_shared.rb:95-96

Current regex:

return unless initializer_content.match?(/^\s*(?\!#).*config\.auto_load_bundle\s*=/) ||
              initializer_content.match?(/^\s*(?\!#).*config\.components_subdirectory\s*=/)

Potential edge cases:

• Won't match if config is inside a conditional block with indentation
• Could match commented lines if # appears after whitespace on same line

Example that might cause issues:

  # config.auto_load_bundle = true  # ← Would incorrectly match
  config.auto_load_bundle = true   # This is fine  # ← Correctly matches

Suggestion (if you want to be more strict):

# Only match lines where config assignment is not in a comment
return unless initializer_content.match?(/^\s*config\.auto_load_bundle\s*=/) ||
              initializer_content.match?(/^\s*config\.components_subdirectory\s*=/)

However, the current implementation is probably fine for 99% of cases since it's unlikely users will have commented config followed by uncommented code with the same pattern.

4. Missing Test Coverage (Important)

Observation: No automated tests for the precompile hook scripts.

Suggestions:

• Add RSpec tests for shakapacker_precompile_hook_shared.rb
• Test scenarios:
  • ReScript detection and build execution
  • Pack generation when configured
  • Rails root detection edge cases
  • Error handling paths
  • Package manager detection

Example test structure:

# spec/react_on_rails/shakapacker_precompile_hook_spec.rb
RSpec.describe "Shakapacker Precompile Hook" do
  describe "#find_rails_root" do
    it "finds Rails root from nested directory"
    it "returns nil when no Rails root found"
  end
  
  describe "#build_rescript_if_needed" do
    it "skips when no ReScript config present"
    it "builds ReScript when config exists"
    it "fails with helpful message when package.json missing"
  end
  
  describe "#generate_packs_if_needed" do
    it "generates packs when auto_load_bundle configured"
    it "skips when no React on Rails initializer"
  end
end

🔒 Security Analysis

✅ No security vulnerabilities found:

• Uses array form for system calls (prevents shell injection)
• No user input directly concatenated into commands
• Proper path validation before file operations
• Exit codes properly handled

🚀 Performance Considerations

✅ Performance is good:

• Shared implementation reduces code duplication (saves parsing time)
• Early returns prevent unnecessary work
• generate_packs_if_stale already has staleness detection
• Generator template is minimal and fast

📋 Best Practices

✅ Follows React on Rails conventions:

• Adheres to CLAUDE.md guidelines
• Uses emojis for user feedback (consistent with codebase)
• Proper error messages with actionable guidance
• Cross-platform considerations

🎯 Recommendations

High Priority:

1. ✅ Fix find_rails_root to use loop instead of while dir \!= "/" for Windows compatibility

Medium Priority:
2. ✅ Replace which with portable package manager detection
3. ⚠️ Add automated tests for the precompile hook

Low Priority:
4. Consider simplifying regex if edge cases aren't a concern

📝 Documentation

✅ Well documented:

• Clear comments in all files
• Links to Shakapacker docs
• Helpful error messages with fixes

✅ Final Verdict

This is a well-implemented PR that follows good software engineering practices. The architecture is sound, error handling is comprehensive, and the code is maintainable.

Main concerns:

1. Windows compatibility issue in find_rails_root (easy fix)
2. Package manager detection could be more portable (easy fix)
3. Missing automated tests (would be valuable but not blocking)

The PR is ready to merge after addressing the Windows compatibility issues. The test coverage gap is a nice-to-have improvement for future work.

Great work on this implementation! 🎉

Review conducted according to CLAUDE.md guidelines

• ✅ Code quality and best practices
• ✅ Potential bugs identified
• ✅ Performance analyzed
• ✅ Security reviewed
• ⚠️ Test coverage gap noted